Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: npm update --save #4223

Merged
merged 4 commits into from
Jan 20, 2022
Merged

fix: npm update --save #4223

merged 4 commits into from
Jan 20, 2022

Conversation

ruyadorno
Copy link
Contributor

@ruyadorno ruyadorno commented Jan 7, 2022 •
edited
Loading

Previously npm update was not respecting the save option, it
would be impossible for users to use npm update and automatically
update their package.json files.

This fixes it by adding extra steps on Arborist.reify._saveIdealTree
to read direct dependencies of any package.json and update them as
needed when reifying using the update and save options.

Also the follow up changes make sure to integrate the original arborist fix
into the cli while better documenting these expectations:

  • Uses config.isDefault to set a different value for the save config
    for both the update and dedupe commands
  • Tweaks arborist to make sure saveIdealTree preserves the behavior of
    skipping writing to package-lock.json on save=false for install while
    still writing the lockfile for npm update with its new default value
    of save=false.
  • Updated and added some new tests on arborist to cover for these tweaks
  • Added npm update --save smoke test on cli

References

Fixes: #708
Fixes: #2704
Relates to: npm/feedback#270

@ruyadorno ruyadorno requested a review from a team as a code owner January 7, 2022 23:10
ruyadorno added a commit to ruyadorno/config that referenced this pull request Jan 10, 2022
@ruyadorno
feat: add isDefault method
7556ce5 
Add a new method `isDefault(key)` that enables a way to know if the
value to be returned by a `config.get(key)` call is coming from the
default definitions or any other different source. In case it's coming
from the default values this method returns `true`, it returns `false`
otherwise.

This addition is going to allow for effectively managing different
default values in the npm cli on a command-basis, e.g:

The `save` config default value in the npm cli is `true`, so that in
`npm install` and other commands that value is always going to default
to true if no user config is provided. Now let's say we want to use a
different value in `npm update`, for example `save=false`. This change
enables us to have a conditional check to see if the `save` config value
is coming from the default source, thus providing a way to use a
different value instead of the default:

    const save = config.isDefault('save') ? false : config.get('save')

Relates to: npm/cli#4223
Relates to: npm/statusboard#324
@ruyadorno ruyadorno mentioned this pull request Jan 10, 2022
Merged
ruyadorno added a commit to npm/config that referenced this pull request Jan 10, 2022
@ruyadorno
feat: add isDefault method (#27)
30ca48d 
Add a new method `isDefault(key)` that enables a way to know if the
value to be returned by a `config.get(key)` call is coming from the
default definitions or any other different source. In case it's coming
from the default values this method returns `true`, it returns `false`
otherwise.

This addition is going to allow for effectively managing different
default values in the npm cli on a command-basis, e.g:

The `save` config default value in the npm cli is `true`, so that in
`npm install` and other commands that value is always going to default
to true if no user config is provided. Now let's say we want to use a
different value in `npm update`, for example `save=false`. This change
enables us to have a conditional check to see if the `save` config value
is coming from the default source, thus providing a way to use a
different value instead of the default:

    const save = config.isDefault('save') ? false : config.get('save')

Relates to: npm/cli#4223
Relates to: npm/statusboard#324
@darcyclarke darcyclarke added semver:patch semver patch level for changes Release 8.x work is associated with a specific npm 8 release cmd:update related to `npm update` release: next These items should be addressed in the next release labels Jan 11, 2022
@ruyadorno
fix: npm update --save
f21ffba 
Previously `npm update` was not respecting the `save` option, it
would be impossible for users to use `npm update` and automatically
update their `package.json` files.

This fixes it by adding extra steps on `Arborist.reify._saveIdealTree`
to read direct dependencies of any `package.json` and update them as
needed when reifying using the `update` and `save` options.

Fixes: npm#708
Fixes: npm#2704
Relates to: npm/feedback#270
@ruyadorno
fix: npm update save=false by default
3cf432a
@ruyadorno
fix: default save config value for update cmd
3bb7bac 
- Uses config.isDefault to set a different value for the `save` config
  for both the update and dedupe commands
- Tweaks arborist to make sure saveIdealTree preserves the behavior of
  skipping writing to package-lock.json on save=false for install while
  still writing the lockfile for `npm update` with its new default value
  of save=false.
- Updated and added some new tests on arborist to cover for these tweaks
- Added `npm update --save` smoke test on cli
@ruyadorno ruyadorno requested review from fritzy and nlf January 12, 2022 03:49
@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Jan 12, 2022
@darcyclarke darcyclarke mentioned this pull request Jan 12, 2022
Closed
@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Jan 13, 2022
@ruyadorno
fix: addressed code review
c0ad0f2 
- added more tests scenarios
- fixed some typos
darcyclarke
darcyclarke approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@darcyclarke darcyclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM

@lukekarrys lukekarrys merged commit cfd59b8 into npm:release-next Jan 20, 2022
@ruyadorno ruyadorno mentioned this pull request Jan 20, 2022
Closed
@lukekarrys lukekarrys mentioned this pull request Jan 20, 2022
Merged
@ruyadorno ruyadorno mentioned this pull request Jan 20, 2022
Closed
@npm-robot npm-robot mentioned this pull request Jan 20, 2022
Closed
@imcotton imcotton mentioned this pull request Jan 22, 2022
Closed
@wraithgar wraithgar mentioned this pull request Mar 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukekarrys lukekarrys lukekarrys approved these changes

@darcyclarke darcyclarke darcyclarke approved these changes

@fritzy fritzy Awaiting requested review from fritzy

@nlf nlf Awaiting requested review from nlf

Assignees
No one assigned
Labels
cmd:update related to `npm update` release: next These items should be addressed in the next release Release 8.x work is associated with a specific npm 8 release semver:patch semver patch level for changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ruyadorno @darcyclarke @lukekarrys